afs firmware with semi-tested apogee detection#2
Conversation
EricPedley
left a comment
EricPedley
left a comment
There was a problem hiding this comment.
Haven't read it all, but left a few comments on the stuff I have read through so far.
|
|
||
| /* Magnetometer data */ | ||
| magData = magnetometer.Read(); | ||
| afsData.magneticFieldX = magData.magneticFieldY; |
There was a problem hiding this comment.
Oh, it's because the orientation of the magnetometer is different then Axis we decided for afs. I'll write a comment for that
| if (afsData.altitude != -1 && altData.altitude != prevAltitude) { | ||
| if (altData.altitude < prevAltitude) { | ||
| tick++; | ||
| // 100 is found based on HZ of loop being around 50 HZ ish |
There was a problem hiding this comment.
This comment is inaccurate now that you changed it to 5, might be confusing to anyone who has to read this later.
There was a problem hiding this comment.
Mb, thanks for the catch
| // 100 is found based on HZ of loop being around 50 HZ ish | ||
| if(tick > 100) | ||
| { | ||
| if (tick > 5) { |
There was a problem hiding this comment.
How fast/slow do we poll the altimeter for us to be confident the metric "five decreasing readings in a row" will never happen because of random noise?
| if ((afsData.altitude != -1) && (altData.altitude != prevAltitude)) { | ||
| // change state after 5 ticks of AFS under 200 ft or 61 meters | ||
| if ((altData.altitude < startAltitude + 61)) { | ||
| tick++; |
There was a problem hiding this comment.
Are you sure you want to reuse the same variable you used for the drogue chute? I feel like it would be easy to introduce a bug like this.
There was a problem hiding this comment.
I reset the variable tho before moving this state. Do you think we need another variable?
| @@ -521,95 +464,97 @@ int main(void) { | |||
| case (0x8): | |||
There was a problem hiding this comment.
Stopped reading here, will come back to it eventually.
|
Also @cadguyen @DanielHeEGG I recommend you download this extension to look at the file diff for this PR since 80% of it is formatting changes. https://mergease.com/ |
2 participants
No description provided.